Skip to content

mcp: inject new-protocol metadata in default send path - #1167

Open
jstar0 wants to merge 2 commits into
modelcontextprotocol:mainfrom
jstar0:fix/client-request-meta-fallback
Open

mcp: inject new-protocol metadata in default send path#1167
jstar0 wants to merge 2 commits into
modelcontextprotocol:mainfrom
jstar0:fix/client-request-meta-fallback

Conversation

@jstar0

@jstar0 jstar0 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Several client-originated requests in the >= 2026-07-28 protocol still relied on individual API methods to attach the per-request _meta fields. That left send paths such as notifications and registered custom methods without protocolVersion, clientInfo, and clientCapabilities, so modern servers treated them as legacy or uninitialized requests.

This changes the default client send handler to clone outgoing params and inject missing request metadata for modern sessions, while keeping the legacy initialize, ping, logging, root-subscription, and resource subscribe compatibility methods on their existing path. CallCustomMethod now relies on the shared send path, so caller params are not mutated just to add metadata before serialization.

Tests:

go test ./mcp -run "Test(ClientNotifyProgressInjectsMetaOnNewProtocol|CallCustomMethodInjectsMetaOnNewProtocol|CallCustomMethodTypedNilParams|ClientConnectDiscover_RequestContents|ClientConnectDiscover_UnsupportedVersionNegotiation|InMemory_E2E_Discover|StreamableClient.*Discover|Streamable.*PerRequest|ValidateRequestMeta|ServerRequest_PerRequestAccessors)" -count=1
go test ./mcp -skip "Test(SSELocalhostProtection|StreamableLocalhostProtection)" -count=1

Full go test ./... currently fails only the existing localhost-protection cases TestSSELocalhostProtection/0.0.0.0_via_localhost_rejects_evil.com and TestStreamableLocalhostProtection/0.0.0.0_via_localhost_rejects_evil.com in this environment; all other packages pass.

Comment thread mcp/client.go
Comment on lines -1677 to -1679
if cs.usesNewProtocol() {
params = injectRequestMeta(cs, params)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not removing this call from all the other client send methods

@jstar0

jstar0 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the question on current head d5d0a9e.

  • Standard client methods no longer call the request-metadata helper themselves; the default sending handler now owns clone-and-inject for every modern non-legacy client request, including registered custom methods.
  • The handler preserves legacy/compatibility wire behavior by converting typed-nil optional params back to an omitted params field, while modern typed-nil params are allocated before metadata injection.
  • List caches now capture cursors before sending, and ReadResource captures its URI before sending, so removing method-level mutation does not introduce nil-parameter panics.

Verification:

  • Focused metadata, typed-nil, discovery, and Streamable HTTP tests pass.
  • Full go test ./... -count=1 passes except the two existing localhost-protection cases for 0.0.0.0 via localhost in this environment; no other failures remain.
  • gofmt and git diff --check pass.

Please review the current head when convenient.

@jstar0
jstar0 force-pushed the fix/client-request-meta-fallback branch from d5d0a9e to 6df1f56 Compare August 20, 2026 11:34
@jstar0

jstar0 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Rebased and pushed the centralized client request metadata injection fix at head 6df1f565 (SSH-signed and verified).

The current head is based on upstream main@b0408f2. Focused metadata, typed-nil, discovery, and Streamable HTTP tests pass; the full suite has no failures beyond the two existing 0.0.0.0 via localhost protection cases in this environment. Hosted checks are running on the new head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants